libroach/engine: plumb support for SingleDelete operation #33440
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 13 of 13 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained
c-deps/libroach/batch.cc, line 89 at r1 (raw file):
break; } case rocksdb::kSingleDeleteRecord:
Can you give more detail on the outcome here? Does this mean that after a SingleDelete, the "deleted" value remains visible? I see that the code is simple in this case, but is this really sane?
c-deps/libroach/batch.cc, line 91 at r1 (raw file):
case rocksdb::kSingleDeleteRecord: // These semantics don't quite mirror SingleDelete's expected // implementation, but they don't violate the its contract from
the its
pkg/storage/engine/rocksdb.go, line 796 at r1 (raw file):
} // Clear removes the most recent item from the db with the given key.
SingleClear
nvanbenschoten
force-pushed
the
nvanbenschoten/singleDelPlumb
branch
from
6492023 to
cfc001a
Compare
There was a problem hiding this comment.
Reviewable status:
c-deps/libroach/batch.cc, line 89 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Can you give more detail on the outcome here? Does this mean that after a SingleDelete, the "deleted" value remains visible? I see that the code is simple in this case, but is this really sane?
Done. It actually means that the SingleDelete is treated like a standard Delete operation (we're in C++, so the fallthrough is implicit).
c-deps/libroach/batch.cc, line 91 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
the its
Done.
pkg/storage/engine/rocksdb.go, line 796 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
SingleClear
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status:
c-deps/libroach/batch.cc, line 89 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Done. It actually means that the SingleDelete is treated like a standard Delete operation (we're in C++, so the fallthrough is implicit).
Ah, makes sense, thanks.
c-deps/libroach/batch.cc, line 92 at r2 (raw file):
// Treating a SingleDelete operation as a standard Delete doesn't // quite mirror SingleDelete's expected implementation, but it // don't violate the operation's contract:
nit: doesn't ;-)
Informs cockroachdb#8979. This commit doesn't actually use the operation yet, but any experiments will require this plumbing, so it made sense to pull it out and properly test it. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/singleDelPlumb
branch
from
cfc001a to
45c072a
Compare
There was a problem hiding this comment.
I really dislike how we translate from Clear -> Delete when going from CRDB to RocksDB. Peer ack for whoever fixes this.
Reviewable status:
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
c-deps/libroach/batch.cc, line 92 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
nit: doesn't ;-)
lol, that don't sound wrong to me... done.
I was talking to @benesch about this today. I guess the idea is that in CRDB code "delete" implies MVCC-awareness (i.e. leaving tombstones) while "clear" doesn't. I don't really like it either so if you're on board I'd happily switch it over. Just give the 👍. |
The current situation is very confusing. |
Merge conflict (retrying...) |
33436: cli/interactive_tests: unskip test_url_db_override r=knz a=knz Fixes #30796. ... and use a more clearly guaranteed-bad DNS name for the expected failure. Suggested by @petermattis. Release note: None 33440: libroach/engine: plumb support for SingleDelete operation r=nvanbenschoten a=nvanbenschoten Informs #8979. This commit doesn't actually use the operation yet, but any experiments will require this plumbing, so it made sense to pull it out and properly test it. Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]>
Build succeeded |
Informs #8979.
This commit doesn't actually use the operation yet, but any experiments
will require this plumbing, so it made sense to pull it out and properly
test it.
Release note: None